Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update account timestamp when login with keycard (#1613) #1614

Merged
merged 1 commit into from
Sep 24, 2019
Merged

Conversation

yenda
Copy link
Contributor

@yenda yenda commented Sep 20, 2019

update account timestamp when login with keycard

Call b.multiaccountsDB.UpdateAccountTimestamp in startNodeWithKey as done in startNodeWithAccount

Closes #1613

@yenda yenda added the blocker label Sep 20, 2019
@yenda yenda self-assigned this Sep 20, 2019
@status-github-bot
Copy link

Hey @yenda, and thank you so much for making your first pull request in status-go! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2

@status-github-bot
Copy link

status-github-bot bot commented Sep 20, 2019

Pull Request Checklist

  • Have you updated the documentation, if impacted (e.g. docs.status.im)?

@status-im-auto
Copy link
Member

status-im-auto commented Sep 20, 2019

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ c2f3cf4 #1 2019-09-20 10:03:37 ~50 sec linux 📦 zip
✔️ c2f3cf4 #1 2019-09-20 10:07:26 ~4 min ios 📦 zip
✔️ c2f3cf4 #1 2019-09-20 10:09:53 ~7 min android 📦 aar
✔️ 84f5b10 #2 2019-09-23 08:47:17 ~34 sec linux 📦 zip
✔️ 84f5b10 #2 2019-09-23 08:51:10 ~4 min ios 📦 zip
✔️ 84f5b10 #2 2019-09-23 08:53:49 ~7 min android 📦 aar

@yenda
Copy link
Contributor Author

yenda commented Sep 20, 2019

Tested in status-react status-im/status-mobile#9014

What is the merging workflow around here?

api/backend.go Outdated

func (b *StatusBackend) StartNodeWithKey(acc multiaccounts.Account, password string, keyHex string) error {
err := b.startNodeWithKey(acc, password, keyHex)
signal.SendLoggedIn(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how significant it is but a potential scenario looks like this :

  1. StartNodeWithKey is called in one thread. startNodeWithKey returns an error.
  2. A signal is received in a different thread.
  3. status-react tries to restart node while b.StopNode() is not finished.

Should be move signal.SendLoggedIn(err) behind b.StopNode()?

Copy link
Contributor Author

@yenda yenda Sep 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

status-react only starts a node via login though, but we could still do that, it would have do be done in StartNodeWithAccount as well though, that is where I took this flow from

@yenda
Copy link
Contributor Author

yenda commented Sep 23, 2019

@adambabik I moved the login signal after stopNode is that what you had in mind?

@adambabik
Copy link
Contributor

@yenda yes!

@yenda
Copy link
Contributor Author

yenda commented Sep 23, 2019

@adambabik cool can you merge it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Account timestamp is not updated when login with keycard
5 participants